Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added a momentum module to si #115

Merged
merged 1 commit into from
Mar 25, 2019

Conversation

dunmatt
Copy link

@dunmatt dunmatt commented Mar 21, 2019

This adds Momentum, per #114 . I'll send another for Torque here in a bit.

@coveralls
Copy link

coveralls commented Mar 21, 2019

Coverage Status

Coverage increased (+0.1%) to 97.077% when pulling ee3810b on dunmatt:buildingMomentum into 372f16e on iliekturtles:master.

Copy link
Owner

@iliekturtles iliekturtles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent start. I noted a few changes to descriptions and abbreviations. I still need to review each unit and conversion factor in detail. I hope to get to that tomorrow.

src/si/momentum.rs Outdated Show resolved Hide resolved
src/si/momentum.rs Outdated Show resolved Hide resolved
src/si/momentum.rs Outdated Show resolved Hide resolved
Copy link
Owner

@iliekturtles iliekturtles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few more changes requested. Before merging I'll want all the commits squashed and the commit message changed to be in the imperative: "Add momentum quantity and associated units.

Resolves #114."

src/si/momentum.rs Outdated Show resolved Hide resolved
src/si/momentum.rs Outdated Show resolved Hide resolved
src/si/momentum.rs Outdated Show resolved Hide resolved
src/si/momentum.rs Outdated Show resolved Hide resolved
src/si/momentum.rs Outdated Show resolved Hide resolved
src/si/momentum.rs Outdated Show resolved Hide resolved
@dunmatt
Copy link
Author

dunmatt commented Mar 24, 2019

Fixed. And, learning from the experience, I've turned on a ruler (in my editor) at the 100 char mark. Thanks for all the feedback! If you don't mind my asking, what's your background? Are you a senior engineer at CERN or something?

@dunmatt dunmatt mentioned this pull request Mar 24, 2019
@iliekturtles
Copy link
Owner

Fixes all look good. Apply the patch below to adjust the where clause formatting and I'll merge!

I am a senior software engineer but don't work for anyone as interesting as CERN. Mostly company-internal modeling work. Also lots of reading for this project.

diff --git a/src/si/momentum.rs b/src/si/momentum.rs
index 122ab2c..9b19e71 100644
--- a/src/si/momentum.rs
+++ b/src/si/momentum.rs
@@ -193,11 +193,13 @@ mod test {
             test::<m::pound, l::foot, t::second, mmm::pound_foot_per_second>();
             test::<m::pound, l::inch, t::second, mmm::pound_inch_per_second>();

-            fn test<M, L, T, R>() where
+            fn test<M, L, T, R>()
+            where
                 M: m::Conversion<V>,
                 L: l::Conversion<V>,
                 T: t::Conversion<V>,
-                R: mmm::Conversion<V> {
+                R: mmm::Conversion<V>
+            {
                 Test::assert_approx_eq(&Momentum::new::<R>(V::one()),
                     &(Mass::new::<M>(V::one()) * Length::new::<L>(V::one())
                         / Time::new::<T>(V::one())));

@dunmatt
Copy link
Author

dunmatt commented Mar 24, 2019

done

@iliekturtles iliekturtles merged commit 54cc327 into iliekturtles:master Mar 25, 2019
@iliekturtles
Copy link
Owner

Merged! Thanks for putting this together.

@dunmatt dunmatt deleted the buildingMomentum branch March 25, 2019 00:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants